Skip to content

Skip the removed ECOS_BB solver in lp_portfolio - #768

Open
xvruipeng-cyber wants to merge 2 commits into
PyPortfolio:mainfrom
xvruipeng-cyber:fix/lp-portfolio-missing-ecos-bb
Open

xvruipeng-cyber wants to merge 2 commits into
PyPortfolio:mainfrom
xvruipeng-cyber:fix/lp-portfolio-missing-ecos-bb

Conversation

@xvruipeng-cyber

Copy link
Copy Markdown

Summary

Fixes #626.

lp_portfolio still selects ECOS_BB whenever the ecos package imports. cvxpy 1.6 stopped registering that solver, so the default allocation raises SolverError: The solver ECOS_BB is not installed even though ecos itself is present. The 1.7.0 plan to drop this default stays in place: ECOS_BB is used only when cvxpy.installed_solvers() still lists it, and the existing FutureWarning is unchanged in that case. Otherwise the call leaves the solver as None and cvxpy picks an installed mixed-integer solver.

Test plan

  • test_lp_portfolio_skips_ecos_bb_when_cvxpy_removed_it: ecos is treated as installed, but ECOS_BB is absent, and Problem.solve receives solver=None
  • test_lp_portfolio_still_defaults_to_ecos_bb_when_installed: when ECOS_BB is listed, the solve still requests it and emits the existing future warning

Made with Cursor

@arhancanli

Copy link
Copy Markdown

I tried to reproduce #626 on both sides of this change (Python 3.12, cvxpy 1.9.3, ecos installed from PyPI, a 3-asset lp_portfolio call with 1000 to spend). In that environment cp.installed_solvers() lists ECOS_BB, and lp_portfolio() returns ({'A': 50, 'B': 15, 'C': 6}, ~2.0) both before and after the patch, so I could not trigger the SolverError here. The guard itself looks right to me (only default to ECOS_BB when cvxpy actually lists it), and the two monkeypatched tests pin both branches.

It would help a reviewer to name the versions that do fail (which cvxpy and which ecos/ecos-bb packages), since with the current ecos wheel cvxpy still registers ECOS_BB. Also, solver=None falls through to whatever cvxpy picks for a mixed-integer problem; in my environment that worked, but a test that runs a real small lp_portfolio with no ecos installed (as in CI without the optional extra) would check that path end to end rather than only the argument passed to solve.

@xvruipeng-cyber

Copy link
Copy Markdown
Author

The failure in #626 is cvxpy 1.6.0 dropping ECOS, which raises SolverError: The solver ECOS_BB is not installed. The old default selected ECOS_BB whenever the ecos package imported, even if cvxpy no longer listed that solver.

On this machine (Python 3.14, cvxpy 1.9.3, ecos not installed) installed_solvers() is CLARABEL, HIGHS, OSQP, SCIPY, SCS. A real two-asset lp_portfolio of 1000 now solves on that path; the new test covers it and skips when ECOS_BB is registered. Your Python 3.12 / cvxpy 1.9.3 run still lists ECOS_BB because the ecos wheel is installed, so the guard keeps the old default and both sides succeed. The three solver tests passed.

@arhancanli

Copy link
Copy Markdown

Thanks for the details. I ran your no-ecos path (Python 3.12, cvxpy 1.9.3, ecos not installed, installed_solvers() = CLARABEL, SCS, SCIPY, HIGHS, OSQP) with a 3-asset lp_portfolio of 1000 on both main and this branch. Both return ({'A': 49, 'B': 15, 'C': 6}, 0.0) with no warning, because on main the ECOS_BB default is only taken when ecos imports, and here it doesn't. I also tried cvxpy 1.6.0 with ecos installed: ECOS_BB is listed, and both sides succeed (the FutureWarning fires on both).

So the new real-solve test passes without the patch in the environment I could build, and I still have not seen the failing combination. The case the guard changes is ecos importable while cp.installed_solvers() lacks ECOS_BB. Which cvxpy and ecos versions produce that for you (the output of pip list | grep -i -E "cvxpy|ecos" from the failing job would settle it)? If the guard is for that case, a test that monkeypatches installed_solvers to omit ECOS_BB while ecos imports (rather than skipping when ECOS_BB is registered) would fail on main and pass here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

SolverError for discrete_allocation / lp_portfolio

2 participants